This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

🤖 This review was generated with Claude Code using Opus 4.8 (1M context). Findings were verified by building the module, running go vet, the test suite, staticcheck, and diffing the vendored proto against the canonical proto/. Treat as reviewer input, not ground truth.

Principal Engineer Review — Go SDK foundation (A)

Reviewed by checking out the branch and reading every non-generated file. go build/go vet are clean and the 130 tests pass at ~73% package coverage. Findings are ordered by severity; all are actionable.

Blocking

1. Dead code will fail the project's own lint gate — sdk/go/openshell/v1/internal/converter/copy.go:51

boolCount is defined but never referenced anywhere in the module (verified across all .go including tests). unused is a default golangci-lint linter, and mise run ci depends on lint. Confirmed with staticcheck:

copy.go:51:6: func boolCount is unused (U1000)

This contradicts the "CI green" claim — mise run lint should be red. Delete boolCount (or wire it into the log-option validation it was presumably written for).

2. Three public Config fields are silent no-ops — sdk/go/openshell/v1/types/config.go:13-15, client.go:64

Config.Timeout, Config.RetryPolicy, and Config.Logger are declared on the public config struct but never read anywhere in the client or connection path (verified by grep). A user who sets Timeout: 30*time.Second or a RetryPolicy gets zero behavioral change, with no error and no doc warning. For the PR that "carries the shared foundation," shipping config knobs that do nothing bakes in an interface people will rely on, and later wiring them up becomes a behavior change. Either implement them (dial/context timeout, gRPC retryPolicy service-config, connection logging) or drop them from this PR and add each alongside its implementation. At minimum, document them as reserved/no-op.

High

3. Vendored proto is hand-edited and drifted from canonical proto/, and proto:sync will clobber the edits — sdk/go/proto/*, sdk/go/mise.toml:204

• UPSTREAM_VERSION pins 29ce6a70…, which is not resolvable in this repo's history — the snapshot isn't reproducible/verifiable from here.
• The vendored openshell.proto has been manually stripped of import "options.proto" and every [(…secret) = true] annotation (because options.proto isn't vendored).
• It has diverged from main: e.g. volume_claim_templates = 9 is still present here but is reserved 9 on main; the newer annotations = 4 request field on main is missing.
• proto:sync does a raw cp "$UPSTREAM_PATH/*.proto", which will re-introduce import "options.proto" and the secret annotations, immediately breaking proto:gen (protoc can't find options.proto).

Net: the generated bindings are built against a stale, hand-modified contract, and the "sync" automation is not idempotent with the manual edits. proto:check only verifies .pb.go matches the local .proto, not that the local .proto matches upstream — so drift is undetected. Recommend vendoring options.proto (or applying a scripted, repeatable transform), making proto:sync reproduce the exact committed state, and adding a check that the vendored proto equals the pinned upstream.

4. Package doc advertises functionality that returns Unimplemented in this PR — sdk/go/openshell/v1/doc.go

The package overview gives copy-paste examples for Exec().Run, Services().Expose, Providers().Profiles(), SSH().CreateSession/Tunnel, TCP().Forward, Config().Update/GetSandbox, and Policy().List (doc.go:37-357). Every one is a stub returning ErrorUnimplemented until PRs B–F. After A merges, pkg.go.dev presents these as working, and a user following the Quick Start past Sandboxes() hits runtime Unimplemented with no compile-time signal. Scope the package doc to what actually works in A, or clearly mark the not-yet-available sections.

Medium

5. internal/grpc/conn.go has zero test coverage — sdk/go/openshell/v1/internal/grpc/conn.go

The connection package ([no test files]) contains the only security-sensitive logic in the PR: TLS default selection, buildTLSCredentials, CA-pool loading, mTLS keypair loading, and the both-CertFile-and-KeyFile invariant. None of it is tested. NewConnection is easily unit-testable (temp cert files; assert error paths for bad CA / half-configured client cert / scheme stripping). Given this is the shared foundation, the crypto path deserves tests now.

6. WatchOptions fields silently ignored — sdk/go/openshell/v1/types/options.go:29-30, sandbox_client.go:174-190

Watch reads only StopOnTerminal; TimeoutSeconds and LabelSelector are never applied. Same silent no-op problem as #2. For a single-object watch, LabelSelector is meaningless — drop it (or document intent), and either honor TimeoutSeconds or remove it in favor of the documented "use context for timeout."

7. EventAdded is defined and re-exported but never emitted — sdk/go/openshell/v1/sandbox_client.go:211

The doc claims the watcher is "Modeled after k8s.io/apimachinery/pkg/watch.Interface," but the initial object and all updates are delivered as EventModified; EventAdded is dead. k8s consumers expect the first delivery to be ADDED. Either emit EventAdded for the first event (the code already handles first separately, so it's a one-line branch) or drop the constant to avoid implying semantics you don't provide.

Low / nits

8. Watch error events can be silently dropped — sandbox_client.go:233-236

The terminal EventError is sent with a non-blocking select { … default: }. If the 64-slot buffer is full (slow consumer), the stream error is dropped and the consumer only sees a closed channel — indistinguishable from clean EOF. Consider a dedicated error field on the watcher, or block on the send guarded by w.done.

9. Watch blocks until the first server event — sandbox_client.go:196

stream.Recv() runs synchronously before Watch returns, so the call blocks (bounded only by ctx) until the gateway produces the first event. Callers reasonably expect Watch to return promptly and stream thereafter. Document it, or move the first Recv into the goroutine.

10. FromGRPCError loses detail and lets non-status errors bypass typing — internal/converter/errors.go:35-52

StatusError.Details is never populated (the field is dead across the SDK), and when status.FromError returns ok=false the raw error is returned unwrapped, so a caller's IsX() checks silently return false for it. The unused Details field is misleading API surface.

11. Unchecked int → uint32 conversions — sandbox_client.go:54,57

uint32(opts[0].Limit) / uint32(opts[0].Offset) truncate on large values (only guarded against negatives by > 0). Not caught by the default linters, but would trip gosec G115 if adopted; cheap to bound-check.

12. Mock server has unsynchronized map access — sandbox_client_test.go:72,102-106,113-117

CreateSandbox, ListSandboxes, and DeleteSandbox touch s.sandboxes without holding s.mu, while GetSandbox/setPhase do lock. -race passes today only because tests don't currently overlap those calls with setPhase goroutines; latent flakiness in the file nominated as the test-pattern exemplar. Lock consistently.

13. The only non-skipped integration test can't pass — integration_test.go:25-34

TestIntegration_HealthCheck calls Health().Check(), which is a stub returning Unimplemented, so against a real gateway require.NoError fails; the others are t.Skip("TODO"). It's build-tagged so normal CI skips it, but as written it's a broken test. Skip it too, or gate on Health landing in PR B.

14. refreshableAuth holds the write lock across the network refresh — auth_refresh.go:83-112

source.Token() (a network call) runs under mu.Lock(), so every concurrent RPC's metadata fetch blocks for the full refresh duration. Correct single-flight behavior, but "coalesced" undersells that it fully serializes callers during refresh. Acceptable; worth a note.

Overall: the structure (typed sub-clients, types/ isolated from proto, converters, typed errors, watch primitives) is sound and the sandbox path is well tested. The items I'd gate merge on are #1 (lint failure), #2 (no-op config fields in the foundation), and #3/#4 (proto-sync integrity + docs overstating current capability); #5 (conn.go tests) is strongly recommended given this PR's role as the base. The rest are cleanups.

My agent's response to #2271 (comment). Most of the things are because of this artificial split to get the PRs down to something more consumable (which was also important as I hight some size limits for code agent's review when I dropped it). But thank you very much for jumping on it, I've addressed the comments (and delayed some until we get the full combo in)

Thanks for the review. Here is my assessment, classifying each finding by root cause:

Already addressed (in a prior fix commit 3b81351):

• Proto drift (finding 3): Synced all protos from canonical proto/ sources and regenerated Go bindings. The options.proto concern is moot: the canonical protos do not import it, so proto:sync will not break proto:gen. The volume_claim_templates field is now correctly reserved 9.
• conn.go untested (finding 5): Added conn_test.go with tests covering all scheme/TLS paths (http:// plaintext, https:// TLS, no-scheme TLS, Insecure config).

Fixed now (commit dab9329):

• Dead boolCount (finding 1): Deleted.
• EventAdded never emitted (finding 7): First watch event now emits EventAdded, subsequent events emit EventModified. Tests updated.
• Mock server races (finding 12): Added mu.Lock/Unlock to all mock server methods accessing s.sandboxes.
• Broken integration test (finding 13): HealthCheck test now t.Skips like the others.
• doc.go scope (finding 4): All sub-client sections not available in PR A are marked "(available in a future release)".
• No-op fields (findings 2, 6): Config.Timeout, RetryPolicy, Logger and WatchOptions.TimeoutSeconds, LabelSelector are documented as "reserved for future use".

Deferred to later PRs (expected from the A-F split):

• Config wiring (finding 2): The actual Timeout/RetryPolicy/Logger implementation requires the full client paths that land in PRs B-F. Documented as reserved for now.
• LabelSelector (finding 6): Meaningless for single-object watch (reviewer agrees). Will revisit when multi-object watch is added.

Accepted as low-priority (not blocking):

• Watch error drop (finding 8): Valid edge case with the 64-slot buffer. Will address if it surfaces in practice.
• Watch blocks on first Recv (finding 9): Documentation issue. The blocking behavior is inherent to the name-to-ID resolution + initial state delivery pattern.
• FromGRPCError detail loss (finding 10): Details field is dead. Will clean up alongside error handling improvements.
• int to uint32 truncation (finding 11): Bounds would only matter at >4B. Low risk but easy to add.
• Refresh lock scope (finding 14): Correct single-flight behavior as noted.

My agent's response to #2271 (comment). Most of the things are because of this artificial split to get the PRs down to something more consumable (which was also important as I hight some size limits for code agent's review when I dropped it). But thank you very much for jumping on it, I've addressed the comments (and delayed some until we get the full combo in)

Sounds good. I figured some of it would be off, but that your agent would sort it out. :)